Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CloudTrail handling of custom policy URLs. #1216

Merged
merged 3 commits into from
Mar 18, 2015

Conversation

danielgtaylor
Copy link
Contributor

This updates the CloudTrail customizations to stop making HTTP requests
to fetch custom policies, instead relying on the updated behavior of the
CLI to automatically handle file and URL parameters for custom commands.

Before this fix, the CLI would fetch the data into a string, then the
CloudTrail customization would try to fetch the string as if it were
a URL, fail, and throw an error.

cc @jamesls @kyleknap

This updates the CloudTrail customizations to stop making HTTP requests
to fetch custom policies, instead relying on the updated behavior of the
CLI to automatically handle file and URL parameters for custom commands.

Before this fix, the CLI would fetch the data into a string, then the
CloudTrail customization would try to fetch the string as if it were
a URL, fail, and throw an error.
@kyleknap
Copy link
Contributor

I like this change. Can you add a test that uses the BaseAWSCommandParamsTest to make sure that paramfile values like file:// notation now works for the cloudtrail commands? I think there is already one test class that uses BaseAWSCommandParamsTest: https://github.com/aws/aws-cli/blob/cloudtrail-policy/tests/unit/customizations/test_cloudtrail.py#L40

To create the custom policy in a local file, you can create a temp file with FileCreator in testutils.py

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 93.37% when pulling 47b285e on cloudtrail-policy into a22da2f on develop.

f.write('{"Statement": []}')
command = (
'cloudtrail create-subscription --s3-use-bucket foo '
'--name bar --s3-custom-policy file://{0}'.format(f.name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Kyle here. Let's verify that the policy we pass into the botocore layer is the value we expect.

@danielgtaylor
Copy link
Contributor Author

@jamesls @kyleknap please take another look. I've updated the test to assert the policy contents are from the file.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 93.4% when pulling 34fd660 on cloudtrail-policy into a22da2f on develop.

@kyleknap
Copy link
Contributor

Thanks! 🚢

@jamesls
Copy link
Member

jamesls commented Mar 17, 2015

:shipit:

danielgtaylor added a commit that referenced this pull request Mar 18, 2015
Fix CloudTrail handling of custom policy URLs.
@danielgtaylor danielgtaylor merged commit 02c5918 into develop Mar 18, 2015
@danielgtaylor danielgtaylor deleted the cloudtrail-policy branch March 18, 2015 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants